Spanify indexed reader classes #380
Conversation
|
@iamcarbon please take a look if you have a chance. |
|
|
||
| GetBytes(index, bytes); | ||
|
|
||
| return encoding.GetString(bytes); |
There was a problem hiding this comment.
If we bump netframework to 462, we may consider adding a polyfill for Encoding.GetString. This would remain allocation free, and simplify ^.
For example:
#if NETFRAMEWORK || NETSTANDARD1_3
internal static class EncodingExtensions
{
public unsafe static string GetString(this Encoding encoding, ReadOnlySpan<byte> bytes)
{
fixed (byte* pBytes = bytes)
{
return encoding.GetString(pBytes, bytes.Length);
}
}
}
#endifThere was a problem hiding this comment.
That's a great approach and we could use it in a few places.
I'll have a think about bumping the target further. Are there other APIs in 462 you know of that'd be good to use?
There was a problem hiding this comment.
Array.Empty is the only other one I've missed so far. That would allow us to kill Util.Empty. It's possible that the lowering of [] may also be improved on net462.
There was a problem hiding this comment.
I'm good with bumping to net462. 4.5.2 went out of support in 2022. 4.6.2 is supported until 2027. I'll file a different PR for that then rebase and update this one.
| Span<byte> bytes = stackalloc byte[8]; | ||
|
|
||
| GetBytes(index, bytes); | ||
|
|
||
| return IsMotorolaByteOrder | ||
| ? BinaryPrimitives.ReadUInt64BigEndian(bytes) | ||
| : BinaryPrimitives.ReadUInt64LittleEndian(bytes); |
| public override byte[] GetBytes(int index, int count) | ||
| { | ||
| ValidateIndex(index, count); | ||
|
|
||
| var bytes = new byte[count]; | ||
| Array.Copy(_buffer, index + _baseOffset, bytes, 0, count); | ||
| return bytes; | ||
| } |
There was a problem hiding this comment.
Good to see the allocating functions killed.
|
Looks great. Just one comment about a future simplification. Also, really great to see the API surface simplified with Spans. |
Allows avoidance of temporary byte arrays, reduces the number of virtual calls to `GetByteInternal`, and uses conversion methods optimised by the framework (e.g. `BinaryPrimitives`).
drewnoakes
force-pushed
the
spanify-indexed-reader
branch
from
ef0dc55 to
3143322
Compare
|
I'll investigate the encoding changes in a separate PR. |
|
@iamcarbon let's get 2.9.0 out soon. If there are any more public API changes you think we'd want, specifically related to performance in response to targeting newer frameworks, let me know. I'm thinking of getting a prerelease package out today. We can also test the new infra for publishing to NuGet by creating releases here on GitHub. |
Originally proposed by @iamcarbon in #380 (comment) This requires allowing unsafe blocks for `net462` and `netstandard1.3`. Co-authored-by: Jason Nelson <[email protected]>
|
Pre-release package is available, per #389 (comment) |
Allows avoidance of temporary byte arrays, reduces the number of virtual calls to
GetByteInternal, and uses conversion methods optimised by the framework (e.g.BinaryPrimitives).